Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

movetest: add test_moving_modules_lazy_import #732

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

movetest: add test_moving_modules_lazy_import #732

wants to merge 6 commits into from

Conversation

SomeoneSerge
Copy link

@SomeoneSerge SomeoneSerge commented Nov 8, 2023 •
edited
Loading

Adds a failing test case for #731:

>       self.assertEqual(expected, self.mod1.read())
E       AssertionError: 'def import_later():\n    import pkg2.pkg3.pkg4.mod4' != 'import pkg2.pkg3.pkg4.mod4\ndef import_la[34 chars]mod4'
E       + import pkg2.pkg3.pkg4.mod4
E         def import_later():
E             import pkg2.pkg3.pkg4.mod4

ropetest/refactor/movetest.py:614: AssertionError
================================================================================================= warnings summary =================================================================================================
ropetest/refactor/movetest.py: 75 warnings
  /home/ss/Sources/rope/rope/base/project.py:229: DeprecationWarning: Delete once deprecated functions are gone
    self._init_source_folders()

Description

Please include a summary of the change and which issue is fixed.

Related: #731 (only partial fix, the extra import is still added)

Checklist (delete if not relevant):

  • I have added tests that prove my fix is effective or that my feature works
  • I have updated CHANGELOG.md

SomeoneSerge
SomeoneSerge commented Nov 9, 2023
View reviewed changes
rope/base/worder.py
@@ -341,7 +353,7 @@ def is_import_statement(self, offset):
line_start = self._get_line_start(last_import)
return (
self._find_import_end(last_import + 7) >= offset
and self._find_word_start(line_start) == last_import
and self._find_next_word_start(line_start) == last_import
Copy link
Author

@SomeoneSerge SomeoneSerge Nov 9, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, this didn't break any tests (the only 3 failing ones are autoimports-related, and they fail on master too), and has fixed the ones I introduced. I'm still not sure I understood the original code correctly though

@SomeoneSerge SomeoneSerge marked this pull request as ready for review November 9, 2023 00:35
@SomeoneSerge
Copy link
Author

I observed some issues with "lazy" from-import statements where the module names sometimes wouldn't get updated. Summarized in the last test I pushed:

E       AssertionError: 'def import_later():\n    from pkg2.pkg3.p[77 chars]hing' != 'import pkg2.pkg3.pkg4.mod4\ndef import_la[109 chars]hing'                                                                     
E       + import pkg2.pkg3.pkg4.mod4                                                                                                                                                                                
E         def import_later():
E       -     from pkg2.pkg3.pkg4 import mod4
E       ?             -----------
E       +     from pkg import mod4
E             from pkg2.pkg3.pkg4.mod4 import thing
E         
E       -     mod4
E       +     pkg2.pkg3.pkg4.mod4
E             thing

ropetest/refactor/movetest.py:639: AssertionError

Where the task was to move pkg.mod4 to pkg2.pkg3.pkg4.mod4

@SomeoneSerge
Copy link
Author

I can rebase, shall I?

@lieryan
Copy link
Member

lieryan commented Jan 11, 2024

Hi @SomeoneSerge, we usually use a merge-based workflow, so a rebase is not usually necessary and I don't usually do a rebase before merging. But if the PR have too many re-merges with master or there are lots of unnecessary intermediate commits that isn't valuable to keep then a rebase may be sensible. Basically, you can and should use your own discretion whether or not to rebase.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@SomeoneSerge @lieryan